Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix ->methods to work with constants (and perl 5.28) #299

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

cpansprout
Copy link

perl5i’s $class->mc->methods method treats constants inconsistently, because constants created by ‘use constant’ are stored as simple scalar refs, whereas constants created via ‘sub foo(){1}’ are stored as subs in glob.

Also, perl5i fails with an optimization that I hope to include in bleadperl soon, more info on which can be found here: https://rt.perl.org/Ticket/Display.html?id=132252#txn-1500037

The cause of both is the same: perl5i assumes that all subs in stashes reside in globs.

Constants created by ‘use constant’ are store in the stash as scalar
references, whereas constants created by sub foo () { 42 } are stored
as subs-in-globs.

Before this commit, $class->mc->methods would return the latter kind
of constant, and skip the former, unless someone at some point hap-
pened to take a reference to one of the former before ->mc->methods
got called.

If we are going to be poking around in stashes like this, then we do
need to be are of what kind of stuff perl sneaks in there.  Constants
should not be treated inconsistently because of the internal rep-
resentation.

Also, if I get my way, most subs will start to be stored in stashes as
simple coderefs (which saves LOTS of memory), in perl 5.28.

This commit kills two stones with one bird: it handles constants cor-
rectly (an existing bug) and gets ready for the optimization that I am
almost certain will make it into perl 5.28.  We don’t need new tests
for the latter, because my optimization makes existing tests fail.

(For more info on the optimization, see
<https://rt.perl.org/Ticket/Display.html?id=132252#txn-1500037> and
also <https://rt.perl.org/Ticket/Display.html?id=129916>.)
@schwern
Copy link
Contributor

schwern commented Nov 2, 2017

Do constants count as methods? I said in #299 that methods should return anything that can be called as a method, but then I looked at the code and noticed it avoids anything with a signature that's not declared as a method.

If we can consistently differentiate between constants and methods, I'd be inclined to not have constants in methods and instead have a constants method. Then maybe a catch-all subs to just blindly return every subroutine.

I'm inclined towards this because it solves a trickier problem than just listing all possible things which could be called as a method. methods would provide a reasonable heuristic to determine what's really a method, and what's just some thing implemented with a subroutine that happens to be callable as a method. Obviously this is a bit of a rabbit hole in Perl, but I think we can give it a go.

I'm inclined to say that if it has a prototype, it isn't a method. But putting prototypes on methods is a common mistake. So I'm inclined to say that if it has a sub () prototype, treat it as a constant. Unless there's a way to directly determine if a subroutine has been constant folded?

What do you think?

@cpansprout
Copy link
Author

cpansprout commented Nov 2, 2017

What do you think?

It sounds good, but I am not sure what you mean by this:

Unless there's a way to directly determine if a subroutine has been constant folded?

Do you mean to see whether a particular subroutine has been inlined as a constant somewhere?

BTW, perl5::i defines a method like this:

use constant imports => ('perl5i::latest');

which I thought was rather neat.

@cpansprout
Copy link
Author

I'm inclined to say that if it has a prototype, it isn't a method. But putting prototypes on methods is a common mistake. So I'm inclined to say that if it has a sub () prototype, treat it as a constant. Unless there's a way to directly determine if a subroutine has been constant folded?

What do you think?

On second thought, sub do_stuff () { ... } with a body that does not just return the same value every time should almost certainly not be returned by a constants method.

We can use B to inspect a sub to see whether it is constant. (Is that want you meant be constant-folded?) Is loading B acceptable? Some people complain that it is too bloated. I would write a tiny sub-inspection XS module.

@schwern
Copy link
Contributor

schwern commented Nov 3, 2017

We can use B to inspect a sub to see whether it is constant. (Is that want you meant be constant-folded?)

Yes. Though I've used constants myself as class methods so... we're back to the dilemma I proposed about AUTOLOAD. I'm thinking the idea of methods returning CODE objects which can be further inspected makes more sense than trying to puzzle out the meaning of a subroutine.

Is loading B acceptable? Some people complain that it is too bloated. I would write a tiny sub-inspection XS module.

B is already loaded by some compile-time dependency, so that's ok. And it's also used by as_json at runtime. Though I'm sure a slimmer B would be appreciated by many. Anyhow, that's an implementation issue we can change as needed.

Father Chrysostomos added 2 commits November 12, 2017 17:13
In 5.28, sub b(){2} now creates a shorthand constant the way ‘use
constant’ has done since 5.10.  So test another, reified constant
sub as well.
This is just a proof of concept, with one method showing how this
could be implemented.
@cpansprout
Copy link
Author

I have updated the pull request with a patch that gives a proof of concept of how ->mc->methods could work, if you want it to return objects that give info about the methods.

I do not intend to do any more work on this, but I have provided a basis that you can take any direction you like.

@schwern
Copy link
Contributor

schwern commented Nov 16, 2017

I like where this is going. The solution of making MethodInfo delegate to CODE is good to resolve the issue that the same code ref might have different names.

A few notes.

There's an old open issue about this, #80. And an old PR, #246. It hasn't been re-examined in a number of years, but it's worth perusing.

This will apply to more than methods, so SubInfo would be a better generic name.

We can use perl5i features in perl5i code, as long as it doesn't get circular. For example lib/perl5i/2.pm makes use of alias. I'll mark them up in line comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants